Add `fastly_acl` hostcalls #438

awilliams-fastly · 2024-10-24T23:24:29Z

This adds support for two new hostcalls:

fastly_acl::open
fastly_acl::lookup

ACLs can be mocked via fastly.toml by adding one or more acls to local_server. Each acl entry is a reference to an external JSON file. Example:

[local_server]
acls.acl-v4 = "./acl-v4.json"
acls.acl-v6 = {file = "./acl-v6.json"}

The JSON file contains a list of one or more ACL entries, in the same format as can be used to modify ACLs via api.fastly.com. Example:

{
  "entries": [
    { "prefix": "1.2.3.0/24", "action": "BLOCK" },
    { "prefix": "192.168.0.0/16", "action": "BLOCK" },
    { "prefix": "23.23.23.23/32", "action": "ALLOW" },
    { "prefix": "1.2.3.4/32", "action": "ALLOW" },
    { "prefix": "127.0.0.0/8", "action": "ALLOW" }
  ]
}

The test-fixtures/src/bin/acl.rs file shows example guest usage.

Integration Tests

The fastly Rust crate containing support for ACLs has not yet been released. Because of that, the acl.rs integration test uses a configuration feature to conditionally compile, with a default of not using the fastly::acl feature:

Viceroy/test-fixtures/src/bin/acl.rs

Lines 5 to 7 in 6aae133

    
           // Temporary until fastly SDK is released which 
        
           // includes the fastly::acl module. 
        
           #[cfg(feature = "acl_hostcalls")]

To test against a local, unreleased crate:

Modify test-fixtures/Cargo.toml to point to the appropriate local crates
Compile with acl_hostcalls feature: (cd test-fixtures && cargo build -F acl_hostcalls --target=wasm32-wasi)

Once a new version of the crate has been published, the conditional can be removed. I have tested against the internal/unreleased crate and verified that it passes.

ulyssa · 2024-10-30T18:11:01Z

test-fixtures/src/bin/acl.rs

+fn main() -> Result<(), Error> {
+    // Temporary until fastly SDK is released which
+    // includes the fastly::acl module.
+    #[cfg(feature = "acl_hostcalls")]


If I'm following right, is the idea here to:

Merge this PR

Publish Viceroy

Publish Rust SDK with ACLs support

Do follow-up PR to update SDK version dependency and remove the acl_hostcalls feature guard

Yes, that's also how I was thinking the sequencing would be.

This feature guard can be removed once the new fastly crate is published (the order of the previous steps shouldn't matter).

ulyssa · 2024-10-30T19:26:44Z

lib/src/config/acl.rs

+#[derive(Clone, Debug, Default)]
+pub struct AclConfig(pub(crate) acl::Acls);
+
+mod deserialization {


It seems fine, but I'm curious: is there a reason to put this into its own deserialization module? And would it be worth putting it into its own lib/src/config/acl/deserialization.rs?

I choose this structure to match other similar sections (e.g backends, device_detection, dictionaries, ...). So not a very compelling reason.

I'll change it to whichever organization you think best.

ulyssa

I think this looks good! I just have two minor questions.

mhp

Thanks for doing this work. I've left a couple of comments regarding the json format, but I think I just convinced myself it was probably fine. I've also left a note to the effect that the goal here is to match production behaviour on a functional basis, not on a performance basis.

mhp · 2024-10-31T12:05:44Z

test-fixtures/data/my-acl-1.json

@@ -0,0 +1,8 @@
+{


I really like the fact that the file formats are close to the format used to send updates to the API. How do you feel about adding the "op" field too, so that the formats are the same?

Yes, good idea. I've made that change in c18e3b6

mhp · 2024-10-31T12:12:15Z

lib/src/acl.rs

+        { "op": "update", "prefix": "192.168.0.0/16", "action": "BLOCK" },
+        { "op": "create", "prefix": "23.23.23.23/32", "action": "ALLOW" },
+        { "op": "update", "prefix": "1.2.3.4/32", "action": "ALLOW" },
+        { "op": "update", "prefix": "1.2.3.4/8", "action": "ALLOW" }


It looks like this testcase includes the "op" field, which is nice. Is it just ignored then? That's probably fine then!

Yes, it's ignored (along with any other extraneous fields).

Thanks for pointing out the inconsistency with this test case and the integration test data. In c18e3b6, I've corrected that along with adding better comments to better document the JSON structure.

mhp · 2024-10-31T12:33:01Z

lib/src/acl.rs

+    /// over the acls entries.
+    ///
+    /// If the IP matches multiple ACL entries, then:
+    /// - The most specific match is returned (longest mask),


This looks like it gives the same results as we'd expect to see in production. Obviously the performance characteristics will be different, but I don't believe that it is a goal of Viceroy to duplicate that.

@ulyssa

This updates the test fixtures to use the newly released `0.11.1` Fastly crate. When `fastly::acl` hostcalls were added in #438, the corresponding SDK support was not yet released. So #438 added tests using the still-unreleased-SDK, but put that code behind a compile-time gate ([here](#438 (comment))). This change removes that compile-time gate in `test-fixtures/src/bin/acl.rs`, effectively changing these tests from a no-op to running the desired test cases. A few other text-fixtures stopped compiling with the updated SDK, which I've tried to address: - `test-fixtures/src/bin/inspect.rs` cc/ @ulyssa - `test-fixtures/src/bin/kv_store.rs` cc/ @joeshaw

This updates the test fixtures to use the newly released `0.11.1` Fastly crate. When `fastly::acl` hostcalls were added in #438, the corresponding SDK support was not yet released. So #438 added tests using the still-unreleased-SDK, but put that code behind a compile-time gate ([here](#438 (comment))). This change removes that compile-time gate in `test-fixtures/src/bin/acl.rs`, effectively changing these tests from a no-op to actually running the desired test cases. A few other text-fixtures stopped compiling with the updated SDK, which I've tried to address as well: - `test-fixtures/src/bin/inspect.rs` - `test-fixtures/src/bin/kv_store.rs` The following hostcalls were missing from the component/adapter, and so were added: - `fastly_http_req#send_v3` - `fastly_http_req#send_async_v2` - `fastly_http_cache` (entire module)

awilliams-fastly force-pushed the awilliams/acls branch from 764443e to 76e6dc9 Compare October 28, 2024 16:21

Add fastly_acl hostcalls

6aae133

awilliams-fastly force-pushed the awilliams/acls branch from 76e6dc9 to 6aae133 Compare October 28, 2024 19:26

awilliams-fastly requested review from ulyssa, mhp and silentbicycle October 28, 2024 20:15

awilliams-fastly marked this pull request as ready for review October 28, 2024 20:15

awilliams-fastly changed the title ~~Add fastly_acl hostcalls~~ Add fastly_acl hostcalls Oct 28, 2024

deg4uss3r mentioned this pull request Oct 30, 2024

Support ACL Hostcalls fastly/js-compute-runtime#1024

Open

ulyssa reviewed Oct 30, 2024

View reviewed changes

ulyssa previously approved these changes Oct 30, 2024

View reviewed changes

mhp previously approved these changes Oct 31, 2024

View reviewed changes

Better document and test the JSON format of ACLs

c18e3b6

awilliams-fastly dismissed stale reviews from mhp and ulyssa via c18e3b6 October 31, 2024 15:28

awilliams-fastly requested review from ulyssa and mhp November 1, 2024 20:52

ulyssa approved these changes Nov 1, 2024

View reviewed changes

awilliams-fastly merged commit eeac2f4 into main Nov 4, 2024
15 checks passed

awilliams-fastly deleted the awilliams/acls branch November 4, 2024 23:01

dgryski mentioned this pull request Nov 15, 2024

acl: add ACL hostcalls fastly/compute-sdk-go#146

Merged

awilliams-fastly mentioned this pull request Dec 5, 2024

Update Fastly crate from 0.10.4 to 0.11.1 #445

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `fastly_acl` hostcalls #438

Add `fastly_acl` hostcalls #438

awilliams-fastly commented Oct 24, 2024 •

edited

Loading

ulyssa Oct 30, 2024 •

edited

Loading

awilliams-fastly Oct 31, 2024

ulyssa Oct 30, 2024

awilliams-fastly Oct 31, 2024 •

edited

Loading

ulyssa left a comment

mhp left a comment

mhp Oct 31, 2024

awilliams-fastly Oct 31, 2024

mhp Oct 31, 2024

awilliams-fastly Oct 31, 2024

mhp Oct 31, 2024

	// Temporary until fastly SDK is released which
	// includes the fastly::acl module.
	#[cfg(feature = "acl_hostcalls")]

Add fastly_acl hostcalls #438

Add fastly_acl hostcalls #438

Conversation

awilliams-fastly commented Oct 24, 2024 • edited Loading

Integration Tests

ulyssa Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

awilliams-fastly Oct 31, 2024

Choose a reason for hiding this comment

ulyssa Oct 30, 2024

Choose a reason for hiding this comment

awilliams-fastly Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

ulyssa left a comment

Choose a reason for hiding this comment

mhp left a comment

Choose a reason for hiding this comment

mhp Oct 31, 2024

Choose a reason for hiding this comment

awilliams-fastly Oct 31, 2024

Choose a reason for hiding this comment

mhp Oct 31, 2024

Choose a reason for hiding this comment

awilliams-fastly Oct 31, 2024

Choose a reason for hiding this comment

mhp Oct 31, 2024

Choose a reason for hiding this comment

Add `fastly_acl` hostcalls #438

Add `fastly_acl` hostcalls #438

awilliams-fastly commented Oct 24, 2024 •

edited

Loading

ulyssa Oct 30, 2024 •

edited

Loading

awilliams-fastly Oct 31, 2024 •

edited

Loading